-
Notifications
You must be signed in to change notification settings - Fork 13.4k
[flang] Add comdats to functions with linkonce linkage #66516
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This fixes a bug where functions generated by the MLIR Math dialect, for example ipowi, would fail to link with link.exe on Windows due to having linkonce linkage but no associated comdat. Adding the comdat on ELF also allows linkers to perform better garbage collection in the binary. Simply adding comdats to all functions with this linkage type should also cover future cases where linkonce or linkonce_odr functions might be necessary.
@llvm/pr-subscribers-flang-fir-hlfir @llvm/pr-subscribers-flang-codegen ChangesThis fixes a bug where functions generated by the MLIR Math dialect, for example ipowi, would fail to link with link.exe on Windows due to having linkonce linkage but no associated comdat. Adding the comdat on ELF also allows linkers to perform better garbage collection in the binary.Simply adding comdats to all functions with this linkage type should -- 3 Files Affected:
diff --git a/flang/lib/Optimizer/CodeGen/CodeGen.cpp b/flang/lib/Optimizer/CodeGen/CodeGen.cpp index 0ec7e570395e89c..6a6a10f632648cd 100644 --- a/flang/lib/Optimizer/CodeGen/CodeGen.cpp +++ b/flang/lib/Optimizer/CodeGen/CodeGen.cpp @@ -34,6 +34,7 @@ #include "mlir/Conversion/ReconcileUnrealizedCasts/ReconcileUnrealizedCasts.h" #include "mlir/Conversion/VectorToLLVM/ConvertVectorToLLVM.h" #include "mlir/Dialect/LLVMIR/LLVMDialect.h" +#include "mlir/Dialect/LLVMIR/Transforms/AddComdats.h" #include "mlir/Dialect/OpenACC/OpenACC.h" #include "mlir/Dialect/OpenMP/OpenMPDialect.h" #include "mlir/IR/BuiltinTypes.h" @@ -3861,6 +3862,14 @@ class FIRToLLVMLowering std::move(pattern)))) { signalPassFailure(); } + + // Run pass to add comdats to functions that have weak linkage on relevant platforms + if (fir::getTargetTriple(mod).supportsCOMDAT()) { + mlir::OpPassManager comdatPM("builtin.module"); + comdatPM.addPass(mlir::LLVM::createLLVMAddComdats()); + if (mlir::failed(runPipeline(comdatPM, mod))) + return signalPassFailure(); + } } private: diff --git a/flang/test/Fir/comdat.fir b/flang/test/Fir/comdat.fir new file mode 100644 index 000000000000000..2f5da505e4903f5 --- /dev/null +++ b/flang/test/Fir/comdat.fir @@ -0,0 +1,41 @@ + +// RUN: fir-opt %s --fir-to-llvm-ir="target=x86_64-unknown-linux-gnu" | FileCheck %s --check-prefixes="CHECK-COMDAT" +// RUN: fir-opt %s --fir-to-llvm-ir="target=x86_64-pc-windows-msvc" | FileCheck %s --check-prefixes="CHECK-COMDAT" +// RUN: fir-opt %s --fir-to-llvm-ir="target=aarch64-apple-darwin" | FileCheck %s --check-prefixes="CHECK-NOCOMDAT" +// RUN: fir-opt %s --fir-to-llvm-ir="target=powerpc64-ibm-aix" | FileCheck %s --check-prefixes="CHECK-NOCOMDAT" + +// CHECK-COMDAT: llvm.func linkonce @fun_linkonce(%arg0: i32) -> i32 comdat(@__llvm_comdat::@fun_linkonce) +// CHECK-NOCOMDAT: llvm.func linkonce @fun_linkonce(%arg0: i32) -> i32 { +func.func @fun_linkonce(%arg0: i32) -> i32 attributes {llvm.linkage = #llvm.linkage<linkonce>} { + return %arg0 : i32 +} + +// CHECK-COMDAT: llvm.func linkonce_odr @fun_linkonce_odr(%arg0: i32) -> i32 comdat(@__llvm_comdat::@fun_linkonce_odr) +// CHECK-NOCOMDAT: llvm.func linkonce_odr @fun_linkonce_odr(%arg0: i32) -> i32 { +func.func @fun_linkonce_odr(%arg0: i32) -> i32 attributes {llvm.linkage = #llvm.linkage<linkonce_odr>} { + return %arg0 : i32 +} + +// CHECK-COMDAT: llvm.mlir.global linkonce constant @global_linkonce() comdat(@__llvm_comdat::@global_linkonce) {addr_space = 0 : i32} : i32 +// CHECK-NOCOMDAT: llvm.mlir.global linkonce constant @global_linkonce() {addr_space = 0 : i32} : i32 { +fir.global linkonce @global_linkonce constant : i32 { + %0 = arith.constant 0 : i32 + fir.has_value %0 : i32 +} + +// CHECK-COMDAT: llvm.comdat @__llvm_comdat { +// CHECK-COMDAT: llvm.comdat_selector @fun_linkonce_odr any +// CHECK-COMDAT: llvm.comdat_selector @fun_linkonce any +// CHECK-COMDAT: llvm.comdat_selector @global_linkonce any +// CHECK-COMDAT: llvm.comdat_selector @global_linkonce_odr any + +// CHECK-NOCOMDAT-NOT: llvm.comdat +// CHECK-NOCOMDAT-NOT: llvm.comdat_selector + + +// CHECK-COMDAT: llvm.mlir.global linkonce_odr constant @global_linkonce_odr() comdat(@__llvm_comdat::@global_linkonce_odr) {addr_space = 0 : i32} : i32 +// CHECK-NOCOMDAT: llvm.mlir.global linkonce_odr constant @global_linkonce_odr() {addr_space = 0 : i32} : i32 { +fir.global linkonce_odr @global_linkonce_odr constant : i32 { + %0 = arith.constant 0 : i32 + fir.has_value %0 : i32 +} \ No newline at end of file diff --git a/flang/test/Intrinsics/math-codegen.fir b/flang/test/Intrinsics/math-codegen.fir index 0af896adf3226fd..62d841253075e80 100644 --- a/flang/test/Intrinsics/math-codegen.fir +++ b/flang/test/Intrinsics/math-codegen.fir @@ -1467,9 +1467,15 @@ func.func private @llvm.powi.f64.i32(f64, i32) -> f64 func.func private @pow(f64, f64) -> f64 //--- exponentiation_integer.fir -// RUN: fir-opt %t/exponentiation_integer.fir --fir-to-llvm-ir="target=x86_64-unknown-linux-gnu" | FileCheck %t/exponentiation_integer.fir +// RUN: fir-opt %t/exponentiation_integer.fir --fir-to-llvm-ir="target=x86_64-unknown-linux-gnu" | FileCheck %t/exponentiation_integer.fir --check-prefixes="CHECK,CHECK-COMDAT" +// RUN: fir-opt %t/exponentiation_integer.fir --fir-to-llvm-ir="target=x86_64-pc-windows-msvc" | FileCheck %t/exponentiation_integer.fir --check-prefixes="CHECK,CHECK-COMDAT" +// RUN: fir-opt %t/exponentiation_integer.fir --fir-to-llvm-ir="target=aarch64-apple-darwin" | FileCheck %t/exponentiation_integer.fir --check-prefixes="CHECK,CHECK-NOCOMDAT" +// CHECK-COMDAT: llvm.comdat_selector @__mlir_math_ipowi_i32 any +// CHECK-NOCOMDAT-NOT: llvm.comdat_selector @__mlir_math_ipowi_i32 any // CHECK: @_QPtest_int4 // CHECK: llvm.call @__mlir_math_ipowi_i32({{%[A-Za-z0-9._]+}}, {{%[A-Za-z0-9._]+}}) : (i32, i32) -> i32 +// CHECK-COMDAT: llvm.func linkonce_odr @__mlir_math_ipowi_i32(%arg0: i32, %arg1: i32) -> i32 comdat(@__llvm_comdat::@__mlir_math_ipowi_i32) +// CHECK-NOCOMDAT: llvm.func linkonce_odr @__mlir_math_ipowi_i32(%arg0: i32, %arg1: i32) -> i32 func.func @_QPtest_int4(%arg0: !fir.ref<i32> {fir.bindc_name = "x"}, %arg1: !fir.ref<i32> {fir.bindc_name = "y"}, %arg2: !fir.ref<i32> {fir.bindc_name = "z"}) { %0 = fir.load %arg0 : !fir.ref<i32> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for fixing this!
This fixes a bug where functions generated by the MLIR Math dialect, for example ipowi, would fail to link with link.exe on Windows due to having linkonce linkage but no associated comdat. Adding the comdat on ELF also allows linkers to perform better garbage collection in the binary. Simply adding comdats to all functions with this linkage type should also cover future cases where linkonce or linkonce_odr functions might be necessary. (cherry picked from commit 5f476b8)
This fixes a bug where functions generated by the MLIR Math dialect, for
example ipowi, would fail to link with link.exe on Windows due to having
linkonce linkage but no associated comdat. Adding the comdat on ELF also
allows linkers to perform better garbage collection in the binary.
Simply adding comdats to all functions with this linkage type should
also cover future cases where linkonce or linkonce_odr functions might
be necessary.